-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert on zero amounts #636
Conversation
99745df
to
10b891e
Compare
10b891e
to
5f143f6
Compare
@@ -522,7 +523,8 @@ contract ERC20PoolPrecisionTest is ERC20DSTestPlus { | |||
assertEq(bucketLpBalance, lenderLpBalance + bidderLpBalance); | |||
} | |||
|
|||
function testDepositTwoLendersSameBucket( | |||
// FIXME: the scaled amounts are always 0 | |||
function _testDepositTwoLendersSameBucket( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EdNoepel can you pls check why the calculated scaledQuoteAmount1
and scaledQuoteAmount2
are always 0? It looks like the test is not properly configured. Same for _testDepositTwoActorSameBucket
above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nm, I think I solved that by setting the amount to the dust limit if lower than
9263e04
to
71568b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think these are all fine changes to make. Most apply to token quantities where passing a 0 is a null op. In one case it's applied to a loop limit which is a somewhat different case, but I don't think it harms anything there either as the caller can always pass a value >0 as well and get more or less identical results. Philosophically I still really don't think this buys anything and just clutters the code, but I don't think it causes any harm either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally happy with this. Just a question regarding borrower updating neutral price, and a removed test.
@@ -462,15 +457,14 @@ contract ERC20PoolBorrowTest is ERC20HelperContract { | |||
borrower: _borrower, | |||
borrowerDebt: expectedDebt, | |||
borrowerCollateral: 50 * 1e18, | |||
borrowert0Np: 448.381722115384615591 * 1e18, | |||
borrowert0Np: 445.838278846153846359 * 1e18, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceivably, borrowing zero (old line 435) was a way to update the borrower's neutral price. What's the new mechanism for doing so? Drawing or repaying 1 wei or whatever the dust limit is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must admit I don't understand the mechanism of updating NP by borrowing 0, can you explain what's the scenario that is used for? Maybe if we need such out of context of draw/repay debt we should expose an updateNP
function for borrowers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of this is rudimentary. A borrower's neutral price is a function of the MOMP and LUP, which change over time. Before a borrower is liquidated, I thought the borrower had the right to restamp their neutral price if conditions were favorable to do so. That's what BorrowerActions
old line 125 was permitting, with the restamp happening on old line 219.
Off topic: I do not understand why the borrower needs a neutral price stamp at all. Seems it would be easier to stamp that on the liquidation upon kick
and calculate it on-the-fly until then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! @mattcushman can you please share your thoughts on this matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for stamping the neutral price on the loan at borrowing is to provide some certainty to the borrower as to at what price they can expect to be liquidated. If we made it entirely dynamic, it would invite kickets to try to lower the dynamic measure of price in such a way to make kicking the loans profitable. We struggled to find a good dynamic measure, but this made the most sense over the lifetime of the loan.
And yes, the borrower should be able to restamp their NP as long as they are fully collateralized. THey can always trivially do so by removing 1 unit of debt or collateral.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, can add such function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattcushman should we allow other parties but borrower to restamp the loan? I think we should as we're currently allowing this as draw / repay debt can be invoked by different actors but want to get your confirmation on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added with e07f6bd please review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per our discussion 08b8943 removes the ability of 3rd parties to restamp Neutral Price of loan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
84e6870
to
08b8943
Compare
…kets (settle only with reserves)
) external returns ( | ||
uint256 newLup_ | ||
) { | ||
address borrowerAddress = msg.sender; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since borrowerAddress
is always msg.sender
, can we just remove that local variable entirely to save a bit of gas (CALLER vs MLOAD: https://stackoverflow.com/questions/64854256/how-much-accessing-msg-sender-costs-is-it-useful-to-store-it-in-a-variable-and)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed with 55f44a1
newLup_ = _lup(deposits_, poolState_.debt); | ||
|
||
uint256 borrowerDebt = Maths.wmul(borrower.t0Debt, poolState_.inflator); | ||
bool isCollateralized = _isCollateralized(borrowerDebt, borrower.collateral, newLup_, poolState_.poolType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugly but might want to also remove borrowerDebt
local variable for gas savings since its only used once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed all local vars with d82eda7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just had a few comments about potential gas optimizations
Please review again with latest changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pool.updateInterest
function to be called by actors wanting to update interest ratePool.stampLoan
function to be called by borrowers for restamping Neutral Price of their loanIPoolBorrowerActions
interface for commonstampLoan
functionPool.stampLoan
to accrue interest and stamp Neutral price of the loanBorrowerActions.stampLoan
to update Neutral Price of the loan: reverts if loan in auction or borrower under collateralizedMoveToSamePrice
error toMoveToSameIndex
InvalidAmount
error, use to revert on calling functions with 0 amounts (add/remove quote tokens and collateral, take auction and take reserve auction, draw / repay debt)contract size with change
vs